Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add nginx retain ip header #924

Closed
wants to merge 1 commit into from
Closed

Add nginx retain ip header #924

wants to merge 1 commit into from

Conversation

gaoyifan
Copy link

@gaoyifan gaoyifan commented Oct 12, 2016 •
edited
Loading

If docker container runs behind a reverse proxy (like CDN), unicorn cannot get real client IP, because nginx overrides HTTP header X-Forwarded-For and X-Real-IP.

NGINX_RETAIN_IP_HEADER parameter let nginx retain X-Forwarded-For and X-Real-IP.

@knight42
Copy link

ping @sameersbn @solidnerd

@solidnerd
Copy link
Collaborator

I will test this feature within a week and will give you feedback after that. I hope it's okay for you.

@solidnerd solidnerd self-assigned this Oct 19, 2016
@knight42
Copy link

@solidnerd Any progress?

@lenovouser
Copy link
Contributor

Seems like a good feature to me 👍

@solidnerd
Copy link
Collaborator

Should this be done for nginx-ssl as well ? Is it a possible test case ?

@lenovouser
Copy link
Contributor

Yep, should be done for the SSL part too IMO

@knight42
Copy link

knight42 commented Nov 5, 2016

@solidnerd
nginx_configure_gitlab_retain_ip_header() is called in nginx_configure_gitlab(), which means it directly modify /etc/nginx/sites-enabled/gitlab, hence I think there is no need for this to be done for nginx-ssl.

@phenomax
Copy link
Contributor

phenomax commented Dec 6, 2016

Do we have any updates here?
I also agree that this PR adds an useful feature to this docker image.

@solidnerd solidnerd removed their assignment Jan 9, 2017
@dmednis
Copy link

dmednis commented Jan 19, 2017

When can we expect this to be merged?
This would be a really useful feature.

@gaoyifan
Copy link
Author

@dmednis You can try this fork. Maintained by @knight42

@3kami3
Copy link
Contributor

3kami3 commented Mar 23, 2017

I tried setting GitLabCE official document, so please also try this PR #1137.

@gaoyifan
Copy link
Author

Agree with @3kami3 😀. I think the implementation in #1137 is safer. And I will close this one.

@gaoyifan gaoyifan closed this Mar 23, 2017
@gaoyifan gaoyifan deleted the patch-1 branch March 23, 2017 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gaoyifan @knight42 @solidnerd @lenovouser @phenomax @dmednis @3kami3